Skip to content

Conversation

@caiolima
Copy link
Contributor

This PR contains backport of #60573 to v24. Since it depends on #60429, it also includes a backport version of this PR as well.

To avoid ABI changes, we are also taking in consideration the changes proposed on #60800 from original V8 PR where Isolate:: GetTotalAllocatedBytes is introduced as an alternative method to get total_allocated_bytes_. This avoids ABI breakage due to changes made on HeapStatistics.

caiolima and others added 2 commits November 21, 2025 07:28
Original commit message:

    [api] Adding total allocated bytes in HeapStatistics

    This change exposes total allocated bytes in v8::HeapStatistics API by
    introducing a new total_allocated_bytes() method that tracks all heap
    allocations since an Isolate creation.

    The implementation adds:
    - uint64_t total_allocated_bytes_ field to HeapStatistics.
    - An atomic total allocation counter is stored in the Heap class.
    - The counter is incremented whenever a RestLab is called. This approach can overestimate the total allocation for cases where the LAB is not fully used, but the leftover compared to the LAB itself is quite small, so it seems tolerable.

    Design doc reference:
    https://docs.google.com/document/d/1O4JPsoaxTQsX_7T5Fz4rsGeHMiM16jUrvDuq9FrtbNM

    Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6996467
    Reviewed-by: Dominik Inführ <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Dmitry Bezhetskov <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#103296}

Refs: v8/v8@fe81545
Co-authored-by: Caio Lima <[email protected]>
PR-URL: nodejs#60429
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#60573
Reviewed-By: theanarkh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
fixing tab error.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch. labels Nov 21, 2025
@richardlau
Copy link
Member

To avoid ABI changes, we are also taking in consideration the changes proposed on #60800 from original V8 PR where Isolate:: GetTotalAllocatedBytes is introduced as an alternative method to get total_allocated_bytes_. This avoids ABI breakage due to changes made on HeapStatistics.

I'm no expert on ABI's, but doesn't that change just shift the breakage to Isolate?

@caiolima
Copy link
Contributor Author

I'm no expert on ABI's, but doesn't that change just shift the breakage to Isolate?

I'm not an expert either, so my understanding might be faulty. IIUC, the addition of new non-virtual methods doesn't break ABI, while changing the layout of HeapStatistics do. Any program that compiles with previous include/v8-isolate.h would have no issues compiling with this new header. However with HeapStatistics layout change there will be a mismatch of structures and it will be problematic.

@caiolima
Copy link
Contributor Author

I'm also considering here that the addition of total_allocated_bytes_ on Heap is fine, since it's not exposed outside V8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants